-
Notifications
You must be signed in to change notification settings - Fork 406
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Do not use test launcher when not needed #960
Do not use test launcher when not needed #960
Conversation
["Make variable"](https://docs.bazel.build/versions/master/be/make-variables.html) substitution. | ||
|
||
Execpath returns absolute path, and in order to be able to construct the absolute path we |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, on second though, I feel like this is something that should be an incompatibility flag. I don't think it's too difficult to have users join pwd
and their value themselves. I'm definitely interested in continuing to use execpath
but also want to have debugger support. Maybe rules_rust
can provide some utility crate for expanding execpath
into absolute paths? Not saying that utility has to be done in this PR just trying to figure out if there's a path to completely abolish the launcher and maintain the ability to use absolute paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're saying that you support my followup PR to remove the launcher alltogether using an incompatible flag, and that this PR is good to go.
Or did you mean that this PR needs an incompatible flag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that there'd be no changes in behavior outside that flag. So unless it's flipped you'd still use the launcher. But perhaps that's more restrictive than necessary. But I wonder if anyone would run into issues in this change or be confused why two seemingly similar tests might behave differently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or did you mean that this PR needs an incompatible flag?
This is my question and I think it does but hard to really make up my mind since my attention is currently divided 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My experience is that people encounter issues and confusion when using the launcher. Except really unlikely situations I don't think we would be breaking people by not using the launcher. Of course if we stop producing absolute paths that can really break people and we should guard it with an incompatible change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you think this PR warrants the incompatible flag, I d make it a different one from the one that removes absolute paths, since they both have different fallout impact and migration stories.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can accept this as a middle ground but hope that we can work quickly together to get a feature flag up. As mentioned in another comment. I think the solution for absolute paths would actually be in Bazel proper and not the rules here.
One situation I can think of is tests which look for paths to files using environment variables. if let Ok(exe) = env::var("MY_BIN") {
Command::new(exe).spawn()
} else {
// In non-test/normal runtime, this will rely on PATH
Command::new("my-bin").spawn()
} In this case I think there are solutions but it can be less convenient. |
Maybe we can move this discussion to slack, as soon as it recovers from the outage? :) The use case you show is a reasonable one, but with the change I have in mind there would still be MY_BIN in the env, it's just that the path won't be absolute. |
Yeah, the issues folks will face is that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This backwards compatible change does 2 things:
testing.TestEnvironment
to populate the environment for therust_test
execution. This is a new discovery that we were not aware of, and it mostly removes most common duties from our test launcher.{pwd}
placeholder expansion needed in environment variable values).What will likely follow is an incompatible change that removes
{pwd}
expansion altogether (after I do the reserach to see if it is used :)